-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(#14283) Fix suntab and aixtab when run as unprivileged user #1025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(#14283) Fix suntab and aixtab when run as unprivileged user #1025
Conversation
Apply the create/do_stuff/delete cycle of a temp file that is explained in tempfile.rb to the suntab and aixtab filetype. This way the temporary file should always be removed. The name of the temporary file was changed so it will be easier to stub this single call in tests - without stubbing tempfile creations in other places (like inside Puppet::Util.execute) at the same time.
Running puppet as an unpriviledged user to manage its own crontab caused two problems under solaris: The uid argument was passed to the execute method to set the correct uid before executing the crontab command. The execute method has a default argumenthash with the key :failonfail set to true. Because the suntab filetype passed it's own hash, failonfail was nil so if crontab completes with a non-zero exit code no error will be raised. This is bad because running execute with :uid not only sets the correct userid in the child process but also tries to run `Process.initgroups` to set the correct supplementary groups of that user. This call will most likely fail as a non priviledged user causing the whole child process to fail. The fix now makes sure that failonfail is passed to the execute method so failures can be handled correctly and uid is only passed if necessary (which is not the case when the target user in the crontab resource is the same user that is running puppet). Since the execute method now raises an exception on non-zero exit codes, we have to handle the case where the crontab file doesn't exist for the user (and return '') versus other fatal errors.
Previously, there were places where a rescued exception's message was not included in the newly raised exception. We also were not preserving the backtrace, which makes debugging more difficult. This commit ensures the original message and backtrace are included in the newly raise exception.
Previously there was trailing whitespace and lines indented 4 spaces. Just reformatted the file, no code changes.
Previously, the suntab's write method was printing the contents of the crontab file to stdout.
lib/puppet/util/filetype.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not unlink? I vaguely remember you telling me that was better here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcarlisle yeah, in ruby 1.8.7, close! can raise Errno::EACCES due to calling the finalizer cleanup routine directly. I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcarlisle actually it was Tempfile#close!, but it's fixed now
Previously the flat file tests were split into two example groups at the beginning and end of the file. This commit just consolidates them into a single example group.
This commit refactors the filetype specs to use `let`, especially the arguments passed to the execute method. It also adds a shared behavior for crontab file types and refactors the suntab type to use it.
Previously, the AIX crontab filetype had the same issue as the Sun one, the `:failonfail` argument was being set to false, so it would not raise an error when crontab returned a non-zero exit code. The only time the filetype raised an exception was if the command output contained the line: 'You are not authorized to use the cron command' This commit changes the AIX crontab filetype to raise an exception if crontab returns a non-zero exit code. If the user doesn't have any crontab entries, then the `#read` method will return an empty string as is done for on Solaris. If the command failed because the user doesn't have permission, then the filetype will raise an exception, as it did before. This commit also adds tests and an AIX crontab fixture.
Previously, we were calling `Tempfile#close!` which under ruby 1.8.7 will call the finalizer's cleanup routine directly. That may raise an exception, e.g. Errno::EACCES, if the process is unable to unlink the file, i.e. Windows. In ruby 1.9, the `Tempfile#close!` method was changed to provide the same semantics as `#close` followed by `#unlink`. Rather than monkey patch Tempfile, we simply call close followed by unlink explicitly.
(#14283) Fix suntab and aixtab when run as unprivileged user
Attempting to manage your own crontab file as an unprivileged user would fail on Solaris and AIX because Puppet::Util.execute was being passed the uid to switch to, but that is unnecessary and not allowed.
Also, the
failonfailoption was not being specified, so we were not reporting when the crontab command exited with non-zero status.